overlord/snapstate: do a minimal sanity check on containers #4464

chipaca · 2018-01-10T09:05:08Z

This introduces validateContainer, that uses snap.Container's
Walk to do a minimal sanity check of the container in question.

With this, a snap that has an obvious problem due to e.g. a missing or
non-executable app, or an unreadable snap.yaml, will fail to
install. The error message will alert the user to contact the snap
developers, and the system's log will contain additional details meant
for the developers themselves.

This introduces validateContainer, that uses snap.Container's Walk to do a minimal sanity check of the container in question. With this, a snap that has an obvious problem due to e.g. a missing or non-executable app, or an unreadable snap.yaml, will fail to install. The error message will alert the user to contact the snap developers, and the system's log will contain additional details meant for the developers themselves.

mvo5

Looks great, thanks for working on this. I put some ideas/suggestions inline. I wonder if we should add a tiny (i.e. not testing everything that was already unit tested, just one broken test snap and we try to install it) spread test as well? This way we could check that the logging does what we expect it does.

mvo5 · 2018-01-10T09:16:05Z

overlord/managers_test.go

+
+	var files [][]string
+	for _, app := range info.Apps {
+		files = append(files, []string{app.Command, ""})


Pardon my ignorance, but why an extra empty string ("") here?

the last argument of MakeTestSnapWithFiles is a [][]string, with the inner list's first element being the filename, and the second being its contents

mvo5 · 2018-01-10T09:18:10Z

overlord/snapstate/check_snap.go

+	}
+
+	path = strings.TrimPrefix(filepath.Clean(path), "/")
+	if strings.HasPrefix(path, "../") {


Hm, some questions: when does this condition happens? And if we don't handle it, is normPath() as a name maybe a bit too generic? I.e. if I call normPath("../foo") the result "" is a bit unexpected.

you can create a snap with an app whose command is ../../../bin/sh, in which case there's nothing for the checker to check.
normPath signals "nothing to do" with the empty string.
maybe i should add a comment :-)

It would be nice if we had a bit of logic that reconstructs the real path knowing two bits of information:

classic flag for the snap

effective mount dir for the snap (which depends on 1)

Then we can construct an absolute path by normalizing whatever relative paths we get here and complete the validation.

mvo5 · 2018-01-10T09:27:15Z

overlord/snapstate/check_snap_test.go

+	c.Check(err, Equals, snapstate.ErrMissingPaths)
+}
+
+func (s *checkSnapSuite) TestValidateContainerSnapYamlBadPermsFails(c *C) {


Pardon my ignorance again, but how is this different from TestValidateContainerEmptyButBadPermFails ? Looks quite similar to me, maybe a comment that highlights the differences? For if it is about permissions of "." vs "meta" vs "meta/snap.yaml" - maybe make it table driven?

i'll take a look

while the error and yaml could be tabled, the main differences are in the little setups. Maybe I should add a comment explaining what it's doing? highlighting differences

chipaca · 2018-01-10T09:46:40Z

overlord/snapstate/check_snap.go

+
+var (
+	ErrBadModes     = errors.New("snap is unusable due to bad permissions; contact develper")
+	ErrMissingPaths = errors.New("snap is unusable due to missing files; contact developr")


typo (will fix)

…dator)

bboozzoo · 2018-01-11T07:03:32Z

overlord/snapstate/check_snap.go

+				// Lstat(), and guard against loops, and ...  huge can of
+				// worms, and as this validator is meant as a developer aid
+				// more than anything else, not worth it IMHO (as I can't
+				// imagine this happening by accident).


OTOH it would be nice to cover a scenario when app.Command symlink is dangling.

Simplistic resolving can be done like this: https://play.golang.org/p/oDwwA5x6lBo Then provide something like snap.Container.ResolveSymlink() which in turn would call unsquashfs -ll <snap> <path>, grab the exact path and the target, accumulate those in walk function as these files may still appear when walking and if they don't have a separate WalkPaths(paths []string, walkfunc) which calls unsquashfs -ll <snap> <paths>. Just a thought, though it looks like quite some work.

the problem I don't want to address (and tried to point out in the comment) is that a symlink in a squashfs snap can look like

lrwxrwxrwx john/john 10 2018-01-11 08:11 ./baz -> qux -> foo -> bar

in -lls; I'd have to do two calls, one for the above (to get the target) and one to just -ls to get the filename, and match them up. Much too error prone for what it's gaining us.

Yes, it would be nicer. I don't think it's worth it (at least for now).

I just realised I need to do something about this anyway. Augh.

chipaca · 2018-01-11T10:42:55Z

in the spread tests I use snap try so this test still works once snap pack fails

bboozzoo

Waiting for fixes

bboozzoo · 2018-01-17T14:44:47Z

overlord/snapstate/check_snap.go

+
+		if needsrx[path] || mode.IsDir() {
+			if mode.Perm()&0555 != 0555 {
+				logger.Noticef("in snap %q: %q should be world-readable and -executable, and isn't: %s", s.Name(), path, mode)


...world-readable and executable...

fair enough (done)

bboozzoo · 2018-01-17T14:53:37Z

snap/squashfs/stat.go

+		if idx < 0 {
+			return nil, errBadPath(raw)
+		}
+		st.path = st.path[:idx]


We're so close to resolving the symlinks now. :)

Random thought:

// package squashfs type Readlinker { Readlink() (string, err) } func (s stat) Sys() { return *s } func (s *stat) Readlink() { if st.Mode & os.ModeSymlink { return s.linksTo, nil } return "", errors.New("not a symlink") }

Then while walking:

err := c.Walk(".", func(path string, info os.FileInfo, err error) error { ... if info.Mode() & os.ModeSymlink { if rd, _ := info.Sys().(squashfs.Readlinker); rd != nil { whereTo, err := rd.Readlink() ... } } ....

it'd have to be something like adding Readlink to the Container interface, and have snapdir and squashfs both implement it, but yeah, something like that is doable. With the existing "have -> in a filename and watch it being hilarious" caveat.

As I said on IRC, if we want to do that I think it's followup material, not for this initial PR (which has already stagnated enough)

…anity-check

codecov-io · 2018-01-17T23:35:58Z

Codecov Report

❗ No coverage uploaded for pull request base (master@987ab47). Click here to learn what that means.
The diff coverage is 90.09%.

@@            Coverage Diff            @@
##             master    #4464   +/-   ##
=========================================
  Coverage          ?   78.19%           
=========================================
  Files             ?      453           
  Lines             ?    32058           
  Branches          ?        0           
=========================================
  Hits              ?    25067           
  Misses            ?     4915           
  Partials          ?     2076

Impacted Files	Coverage Δ
cmd/snap-exec/main.go	`71.42% <ø> (ø)`
overlord/snapstate/snapstate.go	`82.16% <33.33%> (ø)`
snap/squashfs/stat.go	`97.34% <60%> (ø)`
overlord/snapstate/check_snap.go	`86.25% <93.54%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 987ab47...2f7a8ea. Read the comment docs.

mvo5 · 2018-01-18T07:38:10Z

tests/main/validate-container-failures/task.yaml

+    tr -s "\n " " " < error.log | MATCH 'contact developer'
+
+    # give things time to reach the journal
+    sleep 1


(nitpick) you could write a for i in $(seq 120); do if jounralctl -u snapd | grep check_snap; then break; fi; sleep 0.2; done here but probably not worth it.

Aha, I see this was already removed.

chipaca force-pushed the snap-permission-sanity-check branch from bd52a4f to ffcbee1 Compare January 10, 2018 09:07

chipaca changed the title ~~overlord/snapstate: add validateContainer, that uses snap.Container's~~ overlord/snapstate: do a minimal sanity check onn containers Jan 10, 2018

chipaca force-pushed the snap-permission-sanity-check branch from ffcbee1 to 91b4232 Compare January 10, 2018 09:08

chipaca changed the title ~~overlord/snapstate: do a minimal sanity check onn containers~~ overlord/snapstate: do a minimal sanity check on containers Jan 10, 2018

mvo5 reviewed Jan 10, 2018

View reviewed changes

chipaca commented Jan 10, 2018

View reviewed changes

address review feedback, and tweak spread tests (and improve the vali…

09d0b43

…dator)

bboozzoo reviewed Jan 11, 2018

View reviewed changes

bboozzoo requested changes Jan 11, 2018

View reviewed changes

chipaca added 2 commits January 11, 2018 16:00

tests: add a spread test for cotainer validation

9652b83

add tests for symlinks, and fix stat of symlinks in squashfs

3328b78

chipaca force-pushed the snap-permission-sanity-check branch from df27ee1 to 3328b78 Compare January 12, 2018 09:23

bboozzoo reviewed Jan 17, 2018

View reviewed changes

chipaca added 2 commits January 17, 2018 16:17

Merge remote-tracking branch 'upstream/master' into snap-permission-s…

4305dcb

…anity-check

deal with git not dealing with permissions

2f7a8ea

bboozzoo approved these changes Jan 18, 2018

View reviewed changes

mvo5 reviewed Jan 18, 2018

View reviewed changes

mvo5 approved these changes Jan 18, 2018

View reviewed changes

mvo5 merged commit 68f9d1c into snapcore:master Jan 18, 2018

chipaca deleted the snap-permission-sanity-check branch February 7, 2018 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

overlord/snapstate: do a minimal sanity check on containers #4464

overlord/snapstate: do a minimal sanity check on containers #4464

chipaca commented Jan 10, 2018 •

edited

mvo5 left a comment

mvo5 Jan 10, 2018

chipaca Jan 10, 2018 •

edited

mvo5 Jan 10, 2018

chipaca Jan 10, 2018

zyga Jan 10, 2018

mvo5 Jan 10, 2018

chipaca Jan 10, 2018

chipaca Jan 10, 2018

chipaca Jan 10, 2018

bboozzoo Jan 11, 2018

chipaca Jan 11, 2018 •

edited

chipaca Jan 11, 2018

chipaca commented Jan 11, 2018

bboozzoo left a comment

bboozzoo Jan 17, 2018

chipaca Jan 17, 2018

bboozzoo Jan 17, 2018

bboozzoo Jan 17, 2018

chipaca Jan 17, 2018

codecov-io commented Jan 17, 2018

mvo5 Jan 18, 2018

mvo5 Jan 18, 2018

overlord/snapstate: do a minimal sanity check on containers #4464

overlord/snapstate: do a minimal sanity check on containers #4464

Conversation

chipaca commented Jan 10, 2018 • edited

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipaca Jan 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipaca Jan 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipaca commented Jan 11, 2018

bboozzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 17, 2018

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipaca commented Jan 10, 2018 •

edited

chipaca Jan 10, 2018 •

edited

chipaca Jan 11, 2018 •

edited